ci, gha: Add Windows jobs based on Linux image #1398

hebasto · 2023-08-10T08:04:22Z

Solves one item in #1392 partially.

hebasto · 2023-08-10T08:09:46Z

Could you please explicitly enable the actions as follows:

docker/setup-buildx-action@*
docker/build-push-action@*
addnab/docker-run-action@*

?

In the meantime, reviewers can view the builds in my personal repo: https://github.com/hebasto/secp256k1/actions/runs/5818492202.

.github/workflows/ci.yml

real-or-random · 2023-08-10T09:16:23Z

In the meantime, reviewers can view the builds in my personal repo: hebasto/secp256k1/actions/runs/5818492202.

Caching doesn't seem to work properly. I suggest trying to remove the mode=min.

hebasto · 2023-08-10T09:19:29Z

In the meantime, reviewers can view the builds in my personal repo: hebasto/secp256k1/actions/runs/5818492202.

Caching doesn't seem to work properly.

Which job?

real-or-random · 2023-08-10T10:03:55Z

In the meantime, reviewers can view the builds in my personal repo: hebasto/secp256k1/actions/runs/5818492202.

Caching doesn't seem to work properly.

Which job?

Sorry, all jobs, but I meant to include a link to the job. But I think I was wrong. The "Building container" stages takes ~5min, but all of this time seems to be spent downloading from the cache?! I didn't expect that it's that slow.

edit: Fetching from ghcr is similarly slow, ok... https://github.com/hebasto/secp256k1/actions/runs/5783560280/job/15672591894#step:2:54

hebasto · 2023-08-10T10:11:38Z

The "Building container" stages takes ~5min, but all of this time seems to be spent downloading from the cache?!

Correct.

I didn't expect that it's that slow.

That is what I meant in #1396 (comment) :)

real-or-random · 2023-08-10T12:18:19Z

Okay! But do you agree with my observation that GHA cache vs. GHCR doesn't make a significant difference here? Then we can ignore that piece when deciding what to use for Linux tasks.

(Heck, for Linux native tasks with standard gcc and clang, using Docker image at all may be faster... But I'd say let's worry about optimizations later.)

hebasto · 2023-08-10T12:30:17Z

Okay! But do you agree with my observation that GHA cache vs. GHCR doesn't make a significant difference here? Then we can ignore that piece when deciding what to use for Linux tasks.

I agree.

The advantage of GHA cache over GHCR is not requiring write permissions for a workflow.

achow101 · 2023-08-10T13:09:17Z

Updated the actions settings

hebasto · 2023-08-11T00:00:47Z

Reworked. The third-party addnab/docker-run-action has been replaced with own reusable workflow.

hebasto · 2023-08-11T10:02:08Z

@real-or-random

Do you have permissions to re-run workflows and jobs?

real-or-random · 2023-08-11T10:02:23Z

Hm, sad to see that it's already starting to get flaky: https://github.com/bitcoin-core/secp256k1/actions/runs/5827144103/job/15802793632?pr=1398 Perhaps this helps: moby/buildkit#3969

Anyway, I like the changes.

hebasto · 2023-08-11T10:05:38Z

Hm, sad to see that it's already starting to get flaky: bitcoin-core/secp256k1/actions/runs/5827144103/job/15802793632?pr=1398

From my experience, I would estimate the rate of such kind of failure is about 1 (one) per cent of total runs.

.github/workflows/ci.yml

hebasto · 2023-08-11T13:24:07Z

Reworked with using a composite action to avoid environment variables hassles.

Using compression=estargz,force-compression=true for caching as suggested by @real-or-random offline.

hebasto · 2023-08-11T13:36:24Z

The only thing I'm not sure about is that whether PR branches will hit image cache from the master branch.

hebasto · 2023-08-11T15:01:51Z

The only thing I'm not sure about is that whether PR branches will hit image cache from the master branch.

I've just finished this check. It works:

Docker layers caches are re-used from the base/default branch.

real-or-random · 2023-08-11T15:03:48Z

Using compression=estargz,force-compression=true for caching as suggested by @real-or-random offline.

Okay, as @hebasto figured out that doesn't work with the GHA cache backend for now (see https://docs.docker.com/build/cache/backends/#cache-compression). Well fine.

hebasto · 2023-08-11T15:05:50Z

Using compression=estargz,force-compression=true for caching as suggested by @real-or-random offline.

Okay, as @hebasto figured out that doesn't work with the GHA cache backend for now (see docs.docker.com/build/cache/backends/#cache-compression). Well fine.

Dropped.

real-or-random · 2023-08-11T15:13:48Z

This looks pretty clean now. Will perform a closer review soon.

One nit: Perhaps it's now time to rename cirrus.ci to ci.sh.

Some observations:

All the Docker stuff is a bit more complex on GHA than on Cirrus CI, but I still believe Docker is a good idea: It makes it easier to reproduce failures locally, and in principle, it makes it easy to switch between different CI providers. For example, we can easily switch back to Cirrus (self-hosted) runners if we keep the Docker images.
Store/restoring the Docker build cache is still a bit slow. It's a bit sad that downloading the cache takes almost as long as running the actual CI script. But we may be able to improve this later. And even if we don't manage to improve, I think the build times are acceptable. (Moreover, the native Windows builds don't use Docker, so we still get some immediate feedback for the most obvious problems.)

hebasto · 2023-08-11T15:21:07Z

One nit: Perhaps it's now time to rename cirrus.ci to ci.sh.

Done.

real-or-random

ACK mod nits

Do you think it makes sense to retain the Cirrus config commented out, like sometimes done in Core? This would require a bit more work here to make sure it still works now that we split the image.

.github/actions/run-in-docker-action/action.yml

real-or-random · 2023-08-11T15:37:43Z

.github/actions/run-in-docker-action/action.yml

+        for var in "$(echo '${{ toJSON(matrix.configuration.env_vars) }}' | jq -r 'to_entries | .[] | "\(.key)=\(.value)"')"; do
+          echo "$var" >> "$GITHUB_ENV"
+        done
+        echo "MAKEFLAGS=-j$(($(nproc) + 1))" >> "$GITHUB_ENV"


nit: This will probably override MAKEFLAGS if in env_vars. Perhaps it's a bit cleaner to simply prepend MAKEFLAGS with $(($(nproc) + 1)), and do this inside ci.sh?

To keep the scope of this PR limited, is it OK to just set mingw_debian.env.MAKEFLAGS: '-j3'?

UPD. Or rely on the global env.MAKEFLAGS: '-j4'?

Or rely on the global env.MAKEFLAGS: '-j4'?

Implemented.

.github/actions/run-in-docker-action/action.yml

real-or-random · 2023-08-11T15:43:30Z

.github/workflows/ci.yml

+      - run: cat tests.log || true
+        if: ${{ always() }}
+      - run: cat noverify_tests.log || true
+        if: ${{ always() }}
+      - run: cat exhaustive_tests.log || true
+        if: ${{ always() }}
+      - run: cat ctime_tests.log || true
+        if: ${{ always() }}
+      - run: cat bench.log || true
+        if: ${{ always() }}
+      - run: cat config.log || true
+        if: ${{ always() }}
+      - run: cat test_env.log || true
+        if: ${{ always() }}
+      - name: CI env
+        run: env
+        if: ${{ always() }}


Could this be refactored into an action? Or is this overkill?

Could this be refactored into an action? Or is this overkill?

In that case it won't show separated entries in the log.

Ok yes, let's leave it like this for now.

(I'm not happy with this and it should be reworked in a separate PR. I had introduced this printing of files to make sure that we always have all logs, and I think that's a good idea in general. But it's not a good idea to pipe it only to files. We should pipe it to files but also keep stdout/stderr, e.g., using tee. People get confused about this over and over because they can't find the error messages in the failing tasks etc.)

real-or-random · 2023-08-11T15:46:24Z

@MarcoFalke if you want to have a look

.github/workflows/ci.yml

jonasnick

Concept ACK, thanks @hebasto!

After @real-or-random explained this approach to me I was able to understand what happens without doing too much research into the github actions semantics. In general, it would be nice to have some comments that document what happens (e.g. what exactly is the purpose of docker_wine_cache and what it does if the image already exists), but we can add them over time in follow-up PRs.

hebasto · 2023-08-17T13:40:42Z

Converting this PR to a draft for now.

Please review #1401 first.

hebasto · 2023-08-17T20:51:04Z

Rebased.

Ready for review.

This makes sense in the process of moving stuff to GitHub Actions.

hebasto · 2023-08-18T09:58:55Z

Rebased.

real-or-random · 2023-08-18T10:05:28Z

I believe it's possible to shave off several minutes from the tasks that run in Docker by setting the docker build driver to docker, see docker/buildx#107 (comment) ff . This should avoid the long "sending tarball" phases. It can be done by setting driver: docker, see https://github.com/docker/setup-buildx-action#inputs . But let's experiment with this after this PR. :)

real-or-random

ACK 87d35f3

hebasto · 2023-08-18T11:50:47Z

I believe it's possible to shave off several minutes from the tasks that run in Docker by setting the docker build driver to docker, see docker/buildx#107 (comment) ff . This should avoid the long "sending tarball" phases. It can be done by setting driver: docker, see docker/setup-buildx-action#inputs . But let's experiment with this after this PR. :)

Unfortunately, GitHub Actions cache does not work with the docker build driver.

jonasnick

ACK 87d35f3

hebasto mentioned this pull request Aug 10, 2023

ci, gha: Add "x86_64: Linux (Debian stable)" GitHub Actions job #1396

Merged

real-or-random added the ci label Aug 10, 2023

real-or-random reviewed Aug 10, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

hebasto force-pushed the 230810-gha-win branch 2 times, most recently from 2dc7abe to dba39d0 Compare August 10, 2023 23:32

real-or-random reviewed Aug 11, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

hebasto force-pushed the 230810-gha-win branch from dba39d0 to ee8c2d7 Compare August 11, 2023 13:22

hebasto force-pushed the 230810-gha-win branch from ee8c2d7 to 133c4e5 Compare August 11, 2023 15:05

real-or-random reviewed Aug 11, 2023

View reviewed changes

real-or-random mentioned this pull request Aug 11, 2023

ci: Future of CI after Cirrus pricing change #1392

Open

17 tasks

real-or-random reviewed Aug 15, 2023

View reviewed changes

.github/workflows/ci.yml Outdated Show resolved Hide resolved

hebasto force-pushed the 230810-gha-win branch from 9ade574 to 7f69b9a Compare August 15, 2023 17:25

jonasnick reviewed Aug 15, 2023

View reviewed changes

hebasto force-pushed the 230810-gha-win branch from 7f69b9a to 716a009 Compare August 15, 2023 19:52

hebasto mentioned this pull request Aug 15, 2023

ci, gha: Run all MSVC tests on Windows natively #1401

Merged

hebasto marked this pull request as draft August 17, 2023 13:40

hebasto force-pushed the 230810-gha-win branch from 716a009 to eb2a483 Compare August 17, 2023 20:50

hebasto marked this pull request as ready for review August 17, 2023 20:50

hebasto added 3 commits August 18, 2023 10:57

ci, gha: Add Windows jobs based on Linux image

2b6f9cd

ci: Remove Windows tasks from Cirrus CI

d6281dd

ci: Rename cirrus.sh to more general ci.sh

87d35f3

This makes sense in the process of moving stuff to GitHub Actions.

hebasto force-pushed the 230810-gha-win branch from eb2a483 to 87d35f3 Compare August 18, 2023 09:58

real-or-random approved these changes Aug 18, 2023

View reviewed changes

jonasnick reviewed Aug 18, 2023

View reviewed changes

jonasnick merged commit 6b9507a into bitcoin-core:master Aug 18, 2023

hebasto deleted the 230810-gha-win branch August 18, 2023 12:48

ci, gha: Add Windows jobs based on Linux image #1398

ci, gha: Add Windows jobs based on Linux image #1398

Uh oh!

Conversation

hebasto commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

real-or-random commented Aug 10, 2023

Uh oh!

hebasto commented Aug 10, 2023

Uh oh!

real-or-random commented Aug 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Aug 10, 2023

Uh oh!

real-or-random commented Aug 10, 2023

Uh oh!

hebasto commented Aug 10, 2023

Uh oh!

achow101 commented Aug 10, 2023

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

real-or-random commented Aug 11, 2023

Uh oh!

hebasto commented Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

real-or-random commented Aug 11, 2023

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

real-or-random commented Aug 11, 2023

Uh oh!

hebasto commented Aug 11, 2023

Uh oh!

real-or-random left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

real-or-random Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 11, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

real-or-random Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

hebasto Aug 11, 2023

Choose a reason for hiding this comment

Uh oh!

real-or-random Aug 12, 2023

Choose a reason for hiding this comment

Uh oh!

real-or-random commented Aug 11, 2023

Uh oh!

Uh oh!

jonasnick left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Aug 10, 2023 •

edited

Loading

hebasto commented Aug 10, 2023 •

edited

Loading

real-or-random commented Aug 10, 2023 •

edited

Loading

hebasto commented Aug 11, 2023 •

edited

Loading

hebasto Aug 11, 2023 •

edited

Loading